Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Linear implementation of Frechet distance #1199

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Linear implementation of Frechet distance #1199

merged 1 commit into from
Jul 12, 2024

Conversation

gliderkite
Copy link

@gliderkite gliderkite commented Jul 10, 2024 •
edited
Loading

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR is due to the fact that when using the frechet_distance methods on our tools with long routes (200km+) we were not able to run it due to fatal runtime error: stack overflow.

This patch includes a new linear implementation (inspired from here) that removes recursion and allows to avoid the stack overflow fatal errors, while improving overall performances (~70% faster when testing 1000 10km-long routes).

urschrei
urschrei requested changes Jul 10, 2024
View reviewed changes
Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small change request from me, for the purposes of maintainability.

@urschrei
Copy link
Member

Oh and if you could contrive a test that fails using the old algorithm but passes now, that would be useful…

@gliderkite
Copy link
Author

Oh and if you could contrive a test that fails using the old algorithm but passes now, that would be useful…

That would be tricky to do, because we would need to generate a huge route, store it somewhere so that it can be accessed locally and on CI to run tests. If you have suggestions or prior examples on how to do this as part of this repo (not sure if you already use git lfs, or have other tests that read from test resources, eg include_str, etc), I will be happy to add this.

@urschrei
Copy link
Member

Oh and if you could contrive a test that fails using the old algorithm but passes now, that would be useful…

That would be tricky to do, because we would need to generate a huge route, store it somewhere so that it can be accessed locally and on CI to run tests. If you have suggestions or prior examples on how to do this as part of this repo (not sure if you already use git lfs, or have other tests that read from test resources, eg include_str, etc), I will be happy to add this.

Ah, in that case probably not worth the hassle.

@gliderkite
Copy link
Author

Ah, in that case probably not worth the hassle.

I can create a deterministic test where we generate two very big LineString stored in memory to then compute the Frechet distance. We are not interested in the result, just that the test completes without panicking. This test will fail with the current implementation (but pass with the new one). If this sounds good to you I can add it as part of this PR.

@urschrei
Copy link
Member

If this sounds good to you I can add it as part of this PR.

If you have time / capacity, that would be great.

urschrei
urschrei approved these changes Jul 11, 2024
View reviewed changes
Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll leave this open for another day or so in case anyone else wants to chime in.

@urschrei urschrei added this pull request to the merge queue Jul 12, 2024
Merged via the queue into georust:main with commit eadc534 Jul 12, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urschrei urschrei urschrei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gliderkite @urschrei